Add startupProbe result handling to kuberuntime #84279

matthyx · 2019-10-24T07:02:17Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
StartupProbe failure of a container should (by design) trigger a restart of the Pod, as it's the case for livenessProbes.

Which issue(s) this PR fixes:
Fixes #84178

Does this PR introduce a user-facing change?:

NONE

matthyx · 2019-10-24T07:03:50Z

/sig node
/cc @odinuge
/priority important-soon

matthyx · 2019-10-24T07:19:11Z

/this-is-unbearable

k8s-ci-robot · 2019-10-24T07:19:12Z

@matthyx:

In response to this:

/this-is-unbearable

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

odinuge · 2019-10-24T07:37:57Z

Thanks!

From the way I read the docs, the KEP and the changes in this PR, I think the comments in the tests should be updated. Isn't the correct behavior to restart the container when the startupProbe fails (not depending on the result of the liveness probe)?

kubernetes/test/e2e_node/startup_probe_test.go

Lines 88 to 93 in ea4570a

    
           		/* 
        
           			Release : v1.16 
        
           			Testname: Pod liveness probe, using local file, delayed by startup probe 
        
           			Description: A Pod is created with liveness probe that uses ‘exec’ command to cat the non-existent /tmp/health file. Liveness probe MUST fail after startup probe expires. The Pod MUST now be killed and restarted incrementing restart count to 1. 
        
           		*/ 
        
           		framework.ConformanceIt("should be restarted with a exec \"cat /tmp/health\" because startup probe does not delay it long enough [NodeConformance]", func() {

More detailed comment here: #84178 (comment)

odinuge · 2019-10-24T09:08:58Z

pkg/kubelet/kubelet.go

+			klog.V(1).Infof("SyncLoop (container unhealthy - liveness): %q", format.Pod(pod))
+			handler.HandlePodSyncs([]*v1.Pod{pod})
+		}
+	case updateStartup := <-kl.startupManager.Updates():


I think this will "race" (not in the sense of a memory race, but the fact that only one of them will get updates to the channel) against this update mechanism, and ignore Success results:

kubernetes/pkg/kubelet/prober/prober_manager.go

Lines 305 to 310 in 323f99e

func (m *manager) updateStartup() {

update := <-m.startupManager.Updates()

started := update.Result == results.Success

m.statusManager.SetContainerStartup(update.PodUID, update.ContainerID, started)

}

Not sure if that is intended or not..

But the channel isn't shared... one listens from startupManager.Updates and the other from livenessManager.Updates. (maybe I don't get the point)

Hmm. Looks like the fact that they don't share the channel is true, since it is created two startupManagers (in this PR).

https://github.com/matthyx/kubernetes/blob/323f99ea8c4faf44090b7554fbfaad2d25120b65/pkg/kubelet/prober/prober_manager.go#L111-L112

Would the startupManager created in pkg/kubelet/kubelet.go ever run?

I hope so... but I have never really worked on that code...

matthyx · 2019-10-24T09:30:36Z

Isn't the correct behavior to restart the container when the startupProbe fails (not depending on the result of the liveness probe)?

Normally in that test we should have startupProbe succeed very quickly, to make sure livenessProbe executes and fails - causing the restart.

odinuge · 2019-10-24T09:53:18Z

Normally in that test we should have startupProbe succeed very quickly, to make sure livenessProbe executes and fails - causing the restart.

The command is cmd := []string{"/bin/sh", "-c", "sleep 600"}, and the startupProbe checks for the file /tmp/health, and will never succeed. From what I understand from the KEP the container should restart when the Probe fails for the nth time, no matter what the result of the livenessProbe is. However the comment Liveness probe MUST fail after startup probe expires makes me think that if the livenessProbe succeeds, the container should not restart.

So, should a pod like this restart or just start normally:

cmd := []string{"sleep", "inf"}
livenessProbe := &v1.Probe{
  Handler: v1.Handler{
    Exec: &v1.ExecAction{
      Command: []string{"true"},
    },
  },
}
readinessProbe := &v1.Probe{
  Handler: v1.Handler{
    Exec: &v1.ExecAction{
      Command: []string{"true"},
    },
  },
}
startupProbe := &v1.Probe{
  Handler: v1.Handler{
   Exec: &v1.ExecAction{
      Command: []string{"false"},
    },
  },
}

matthyx · 2019-10-24T12:06:19Z

should a pod like this restart or just start normally

restart and crash loop backoff

matthyx · 2019-10-24T12:14:32Z

Isn't the correct behavior to restart the container when the startupProbe fails

Actually the difference between the two tests is the FailureThreshold of startupProbe... in the first case it is very big (60) to make sure we actually inhibit livenessProbe during the short lifespan of the test.
The second case has a bug, because I would expect startupProbe to succeed, allowing livenessProbe to run and fail, killing the container.
I think I should use /bin/true and /bin/false to make that more clear and correct.

matthyx · 2019-10-24T12:34:36Z

@odinuge thanks for the comments on e2e tests... here is a PR to fix the ambiguities:
#84291

mattjmcnaughton

/uncc

Deferring to @odinuge, who is already doing a great job with the review :)

matthyx · 2019-10-28T10:41:52Z

/test pull-kubernetes-e2e-kind
/test pull-kubernetes-kubemark-e2e-gce-big

matthyx · 2019-10-31T07:00:02Z

Squashed and ready for review! Thanks a lot for all your help @odinuge !
/assign @Random-Liu

odinuge

Thanks for the work @matthyx! Hopefully we can get this into beta in v1.17!

/lgtm

odinuge · 2019-10-31T10:36:28Z

pkg/kubelet/prober/results/results_manager.go

+	Failure Result = 1
+
+	// Unknown is encoded as -1 (type Result)
+	Unknown Result = -1


Ill defer the naming of this to the approvers (eg. Pending)

odinuge · 2019-10-31T10:38:09Z

pkg/kubelet/prober/results/results_manager.go

+	Failure Result = 1
+
+	// Unknown is encoded as -1 (type Result)
+	Unknown Result = -1
 )

 func (r Result) String() string {


Should we add the new Result type here (and in ToPrometheusType) as well? I think the current behavior is ok, reporting -1/"UNKNOWN", but it may be nice to be explicit. Ill defer that to the approvers 😄

matthyx · 2019-11-05T22:22:12Z

/assign @tallclair

matthyx · 2019-11-12T15:45:14Z

@Random-Liu, @dchen1107, @derekwaynecarr, @tallclair, @vishh, @yujuhong this PR is really needed for 1.17, can you please make sure we don't miss code-freeze?

Random-Liu · 2019-11-12T23:24:23Z

pkg/kubelet/kuberuntime/kuberuntime_manager_test.go

@@ -732,6 +733,7 @@ func TestComputePodActions(t *testing.T) {
 		mutatePodFn    func(*v1.Pod)
 		mutateStatusFn func(*kubecontainer.PodStatus)
 		actions        podActions
+		resetStatusFn  func(*kubecontainer.PodStatus)


Can we createTestRuntimeManager inside the loop? So that we don't need to reset the status.

Makes sense... maybe it could go into another PR?

Random-Liu · 2019-11-12T23:29:32Z

pkg/kubelet/prober/results/results_manager.go

-	// Success is encoded as "true" (type Result)
-	Success Result = true
+	// Success is encoded as 0 (type Result)
+	Success Result = 0


The startup probe logic is almost the same with liveness probe, right?

Why Success/Failure is sufficient for liveness probe, but not startup probe?

OK. Because the init values of liveness probe and startup probes are different.

Given that Unknown is newly introduced, and only used for initializing startup probe, I think this should be safe.

dchen1107 · 2019-11-12T23:31:10Z

/lgtm with one small concern. Asked @yujuhong take a quick look. If she is fine with it, I will approve it.

yujuhong · 2019-11-12T23:32:19Z

pkg/kubelet/prober/worker_test.go

@@ -21,7 +21,7 @@ import (
 	"testing"
 	"time"

-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"


remove "v1"

yujuhong · 2019-11-12T23:34:07Z

pkg/kubelet/prober/results/results_manager.go

+	Failure Result = 1
+
+	// Unknown is encoded as -1 (type Result)
+	Unknown Result = -1


nit: just use iota?

const ( Unknown Result = iota -1 Success Failure )

odinuge

/lgtm

matthyx · 2019-11-13T09:53:07Z

@dchen1107 I think you can approve when you're back!

dchen1107 · 2019-11-13T17:16:14Z

/lgtm
/approve

k8s-ci-robot · 2019-11-13T17:17:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dchen1107, matthyx

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/kubelet/OWNERS~~ [dchen1107]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

matthyx · 2019-11-13T21:10:02Z

Thanks @dchen1107

k8s-ci-robot requested a review from odinuge October 24, 2019 07:03

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 24, 2019

k8s-ci-robot requested review from feiskyer and mattjmcnaughton October 24, 2019 07:03

matthyx force-pushed the kuberuntime-startupprobe branch 3 times, most recently from d51c86c to ddd5eec Compare October 24, 2019 07:18

odinuge reviewed Oct 24, 2019

View reviewed changes

matthyx force-pushed the kuberuntime-startupprobe branch from ddd5eec to 440c44e Compare October 24, 2019 09:26

mattjmcnaughton reviewed Oct 24, 2019

View reviewed changes

matthyx force-pushed the kuberuntime-startupprobe branch from 5e51f45 to 89d4705 Compare October 31, 2019 06:58

k8s-ci-robot assigned Random-Liu Oct 31, 2019

odinuge approved these changes Oct 31, 2019

View reviewed changes

k8s-ci-robot assigned odinuge Oct 31, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 31, 2019

k8s-ci-robot assigned tallclair Nov 5, 2019

matthyx mentioned this pull request Nov 6, 2019

Add pod-startup liveness-probe holdoff for slow-starting pods kubernetes/enhancements#950

Closed

Random-Liu reviewed Nov 12, 2019

View reviewed changes

yujuhong reviewed Nov 12, 2019

View reviewed changes

Random-Liu mentioned this pull request Nov 13, 2019

Clarify startupProbe e2e tests #84291

Merged

Add startupProbe result handling to kuberuntime

66595d5

matthyx force-pushed the kuberuntime-startupprobe branch from 89d4705 to 66595d5 Compare November 13, 2019 07:13

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 13, 2019

odinuge approved these changes Nov 13, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 13, 2019

k8s-ci-robot assigned dchen1107 Nov 13, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 13, 2019

k8s-ci-robot merged commit a08b09d into kubernetes:master Nov 13, 2019

k8s-ci-robot added this to the v1.17 milestone Nov 13, 2019

matthyx deleted the kuberuntime-startupprobe branch November 13, 2019 21:26

matthyx mentioned this pull request Nov 14, 2019

Automated cherry pick of #84279: Add startupProbe result handling to kuberuntime #85262

Closed

spiffxp mentioned this pull request Dec 10, 2019

[Failing Test] [sig-node] startup_probe_test.go #82747

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add startupProbe result handling to kuberuntime #84279

Add startupProbe result handling to kuberuntime #84279

matthyx commented Oct 24, 2019

matthyx commented Oct 24, 2019

matthyx commented Oct 24, 2019

k8s-ci-robot commented Oct 24, 2019

odinuge commented Oct 24, 2019

odinuge Oct 24, 2019

matthyx Oct 24, 2019

odinuge Oct 24, 2019

matthyx Oct 24, 2019

matthyx commented Oct 24, 2019

odinuge commented Oct 24, 2019 •

edited

matthyx commented Oct 24, 2019

matthyx commented Oct 24, 2019

matthyx commented Oct 24, 2019

mattjmcnaughton left a comment

matthyx commented Oct 28, 2019

matthyx commented Oct 31, 2019

odinuge left a comment

odinuge Oct 31, 2019

odinuge Oct 31, 2019

matthyx commented Nov 5, 2019

matthyx commented Nov 12, 2019

Random-Liu Nov 12, 2019

matthyx Nov 13, 2019

Random-Liu Nov 12, 2019

Random-Liu Nov 12, 2019

Random-Liu Nov 13, 2019

dchen1107 commented Nov 12, 2019

yujuhong Nov 12, 2019

yujuhong Nov 12, 2019

odinuge left a comment

matthyx commented Nov 13, 2019

dchen1107 commented Nov 13, 2019

k8s-ci-robot commented Nov 13, 2019

matthyx commented Nov 13, 2019

	func (m *manager) updateStartup() {
	update := <-m.startupManager.Updates()

	started := update.Result == results.Success
	m.statusManager.SetContainerStartup(update.PodUID, update.ContainerID, started)
	}

Add startupProbe result handling to kuberuntime #84279

Add startupProbe result handling to kuberuntime #84279

Conversation

matthyx commented Oct 24, 2019

matthyx commented Oct 24, 2019

matthyx commented Oct 24, 2019

k8s-ci-robot commented Oct 24, 2019

odinuge commented Oct 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthyx commented Oct 24, 2019

odinuge commented Oct 24, 2019 • edited

matthyx commented Oct 24, 2019

matthyx commented Oct 24, 2019

matthyx commented Oct 24, 2019

mattjmcnaughton left a comment

Choose a reason for hiding this comment

matthyx commented Oct 28, 2019

matthyx commented Oct 31, 2019

odinuge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthyx commented Nov 5, 2019

matthyx commented Nov 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dchen1107 commented Nov 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odinuge left a comment

Choose a reason for hiding this comment

matthyx commented Nov 13, 2019

dchen1107 commented Nov 13, 2019

k8s-ci-robot commented Nov 13, 2019

matthyx commented Nov 13, 2019

odinuge commented Oct 24, 2019 •

edited